Skip to content

feat(contract): auto-expire unused launcher image hashes - #3564

Open
barakeinav1 wants to merge 41 commits into
mainfrom
feat/auto-remove-launcher-hashes
Open

feat(contract): auto-expire unused launcher image hashes#3564
barakeinav1 wants to merge 41 commits into
mainfrom
feat/auto-remove-launcher-hashes

Conversation

@barakeinav1

@barakeinav1 barakeinav1 commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Implements the approved design (docs/design/auto-remove-launcher-hashes-design.md, PR #3488).

Usage-based expiry for allowed_launcher_image_hashes, replacing unanimous-vote-only removal for routine cleanup.

Model. Each AllowedLauncherImage stores an expires_at: Timestamp, stamped now + TTL at write time (vote-in, re-vote, and on a successful attestation). An entry is expired when expires_at < now. Because expiry is fixed at write time — mirroring how attestations store their own expiry — reads are a plain comparison and the TTL only touches the write sites; it isn't threaded through the ~20 read/verify paths.

  • Refresh on use — a successful attestation restamps expires_at = now + TTL, but only for a current participant (enforced by requiring an AuthenticatedParticipantId capability token), so a prospective/non-participant node cannot keep a launcher alive. Applies to both attestation paths: the mock path (MockAttestation::WithConstraints may reference a launcher) and the Dstack path (refresh runs in the async resolve_verification callback; the signer is preserved across the verifier promise). No node-side changes.
  • Read-time filtering — all reads of the allowed set skip expired entries; newest-entry fallback so the set is never empty.
  • Inline evictionreverify_and_cleanup_participants (the body of verify_tee) evicts expired entries inline, right after the analogous cleanup of the MPC docker-image hashes; the allowed set is a small in-memory Vec (retain, no separate receipt) and never removes the last entry. No #[private] method, no extra config field.
  • Re-vote recoveryvote_add_launcher_hash on an already-present hash restamps its expires_at (threshold vote), recovering a never-adopted hash.
  • Config — new launcher_hash_unused_ttl_seconds (default 14d), validated >= DEFAULT_EXPIRATION_DURATION_SECONDS (the attestation validity window). Validation is folded into the DTO→Config TryFrom, so init / init_running / update_config can't skip it.

Migration

Shadows the live 3.13.0 baseline in v3_13_0_state.rs: OldConfig deserializes the old config and defaults the new fields; OldTeeState deserializes launcher entries without a timestamp and stamps expires_at = migration_time + TTL. Only Config and allowed_launcher_images changed borsh layout; every other field reuses the real (byte-identical) type. Combined with main's #3785, the same migration also stamps an expiry on legacy MockAttestation::Valid entries so they become cleanable — both steps run together and are covered by tests (including a combined round-trip).

Tests

Expiry filtering, newest fallback, cleanup_expired, re-vote refresh; expired-launcher rejection end-to-end; refresh gating (participant vs. non-participant, both mock and dstack arms); migration round-trips (launcher, legacy-mock, and combined).

Follow-ups

Closes #3381

Comment thread crates/contract/src/tee/proposal.rs Outdated
pub(crate) launcher_hash: LauncherImageHash,
pub(crate) compose_hashes: Vec<LauncherDockerComposeHash>,
pub(crate) added: Timestamp,
pub(crate) last_attested: Timestamp,

@barakeinav1 barakeinav1 Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated (superseded by the last_used refactor in c435dc5):

last_used is a single keep-alive signal — stamped when the hash is voted in / re-voted, and refreshed on each attestation by a current participant. An entry is expired when last_used + TTL < now; the all-expired fallback keeps the most-recently-used entry.

(Originally this was two fields, added + last_attested, with expiry on max(...). Collapsed to one per Patrick's suggestion — the distinction was not load-bearing.)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a feeling it's an overkill to have both in. I feel you only need the last_attested and potentially a bool to state whether it should benefit from full grace or partial one. Then this last_attested is as done currently updated along with the boolean. The expiry would be if bool=true then check last_attested<full grace period , otherwise check last_attested < partial_grace.

Of course this is assuming you have two different grace periods. If you have only one then the logic is even simpler

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed already — collapsed to a single last_used in c435dc5 (we have one TTL / one grace period, so no bool needed). Sorry for the churn on this thread.

@barakeinav1
barakeinav1 force-pushed the feat/auto-remove-launcher-hashes branch from ba05472 to b2d72fe Compare July 2, 2026 10:31
Launcher image hashes accumulated forever; removal required a unanimous
vote. This adds usage-based expiry:

- AllowedLauncherImage gains added/last_attested timestamps; an entry is
  expired when max(added, last_attested) + TTL < now
- last_attested is refreshed on a successful attestation, but ONLY for a
  current participant (enforced by requiring an AuthenticatedParticipantId);
  a prospective/non-participant node cannot keep a stale launcher alive
- reads filter out expired entries, with a newest-entry fallback so the
  allowed set never goes empty
- verify_tee spawns a detached self-call to a new #[private]
  clean_expired_launcher_hashes that sweeps expired entries from storage
- re-voting an existing launcher hash refreshes its added timestamp
- new config launcher_hash_unused_ttl_seconds (default 14d), validated
  >= DEFAULT_EXPIRATION_DURATION_SECONDS
- state migration (v3_12_0_state) initializes timestamps for existing entries

Updates the design doc status to Implemented and regenerates the borsh-schema
and ABI snapshots for the new fields/method.

Closes #3381
@barakeinav1
barakeinav1 force-pushed the feat/auto-remove-launcher-hashes branch from b2d72fe to 99f6904 Compare July 2, 2026 11:32
@barakeinav1
barakeinav1 marked this pull request as ready for review July 2, 2026 11:46
Copilot AI review requested due to automatic review settings July 2, 2026 11:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements the approved “auto-expire unused launcher image hashes” design by adding usage-based expiry for allowed launcher images in the contract, including config/ABI updates, state migration for the new borsh layout, and test coverage for refresh/expiry/cleanup behavior.

Changes:

  • Add added / last_attested timestamps to launcher allowlist entries and filter expired entries at read/verify time (with newest-entry fallback).
  • Refresh last_attested on successful participant submissions and add a detached private cleanup self-call to physically evict expired entries.
  • Introduce new config knobs (launcher_hash_unused_ttl_seconds, clean_expired_launcher_hashes_tera_gas) with interface + DTO mapping + snapshots + migration/tests updated.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
docs/design/auto-remove-launcher-hashes-design.md Marks the design as implemented and updates wording/invariants/decisions to match the shipped behavior.
crates/test-utils/src/contract_types.rs Extends dummy config builder with the new TTL + cleanup gas fields.
crates/near-mpc-contract-interface/src/types/config.rs Adds the new config fields to InitConfig/Config and updates serialization tests.
crates/near-mpc-contract-interface/src/method_names.rs Adds the clean_expired_launcher_hashes method name constant.
crates/mpc-attestation/src/attestation.rs Exposes the launcher compose hash from a verified attestation to support refresh-on-use.
crates/contract/tests/snapshots/abi__abi_has_not_changed.snap Updates ABI snapshot for the new private cleanup method and config fields.
crates/contract/tests/sandbox/upgrade_from_current_contract.rs Updates sandbox upgrade test config to include the new TTL + cleanup gas fields.
crates/contract/tests/sandbox/contract_configuration.rs Updates sandbox init config test to include the new TTL + cleanup gas fields.
crates/contract/src/v3_12_0_state.rs Adds 3.12.0 shadow types for launcher allowlist migration and a migration round-trip test.
crates/contract/src/tee/tee_state.rs Threads launcher TTL through verification paths and adds refresh-on-use + cleanup hooks.
crates/contract/src/tee/proposal.rs Implements TTL filtering, fallback selection, refresh-on-use, re-vote refresh, and cleanup for launcher allowlist entries (with tests).
crates/contract/src/snapshots/mpc_contract__tests__mpc_contract_borsh_schema_has_not_changed.snap Updates borsh schema snapshot for new fields.
crates/contract/src/lib.rs Wires TTL into submit/verify/read paths, spawns detached cleanup self-call, adds private cleanup endpoint, and validates config updates.
crates/contract/src/dto_mapping.rs Maps new config fields between DTOs and contract config.
crates/contract/src/config.rs Adds new config fields, defaults, and a validation invariant tying TTL to attestation expiry window.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 2407 to +2412
pub fn update_config(&mut self, config: dtos::Config) {
self.config = config.into();
let new_config: Config = config.into();
if let Err(e) = new_config.validate() {
env::panic_str(e);
}
self.config = new_config;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed in d7c0c65. init and init_running now build the config, call config.validate(), and propagate the error (both are #[handle_result]), so the contract can no longer be initialized with a TTL below the attestation validity window. Added init_rejects_launcher_ttl_below_attestation_validity to lock it in.

Comment thread crates/test-utils/src/contract_types.rs Outdated
@claude

claude Bot commented Jul 2, 2026

Copy link
Copy Markdown

Pull request overview

Implements usage-based expiry for allowed_launcher_image_hashes: each AllowedLauncherImage now carries added / last_attested timestamps, all reads filter by max(added, last_attested) + TTL >= now, submit_participant_info refreshes last_attested when the caller is authenticated as a current participant, and verify_tee spawns a detached self-call to a new #[private] clean_expired_launcher_hashes for physical eviction. Includes a 3.12.0 shadow borsh layout in v3_12_0_state.rs to migrate existing entries (stamping both timestamps to now).

Changes:

  • New AllowedLauncherImage.{added, last_attested} + is_expired / last_active helpers; AllowedLauncherImages::{live_indices, refresh_last_attested, cleanup_expired, from_entries} and add() semantics changed (re-adding an existing hash now refreshes added and returns true).
  • TeeState::refresh_launcher_usage gated by AuthenticatedParticipantId; add_participant, reverify_participants, reverify_and_cleanup_participants, clean_invalid_attestations, get_allowed_launcher_* all take a new launcher_unused_ttl: Duration.
  • New Config.launcher_hash_unused_ttl_seconds (default 14d) + clean_expired_launcher_hashes_tera_gas (default 5 Tgas); Config::validate() enforces TTL ≥ DEFAULT_EXPIRATION_DURATION_SECONDS, called from update_config.
  • New #[private] clean_expired_launcher_hashes; detached self-call from verify_tee.
  • VerifiedAttestation::launcher_compose_hash() accessor.
  • v3_12_0_state.rs gains OldTeeState / OldAllowedLauncherImage(s) shadow; migration stamps added = last_attested = now.
  • Snapshot updates (borsh schema, ABI) and design-doc status flip to Implemented; docstrings say "participant attestation".

Reviewed changes

Per-file summary
File Description
crates/contract/src/config.rs Adds two config fields (TTL, gas) with defaults and a Config::validate() invariant check
crates/contract/src/dto_mapping.rs Wires the two new config fields through init/get/update DTOs
crates/contract/src/lib.rs Threads launcher_unused_ttl into every TEE call site; refreshes on use gated by AuthenticatedParticipantId; detached sweep from verify_tee; new #[private] clean_expired_launcher_hashes; validation now called by update_config
crates/contract/src/tee/proposal.rs AllowedLauncherImage gains timestamps + is_expired; AllowedLauncherImages gains live_indices / refresh_last_attested / cleanup_expired / from_entries; add() now refreshes an existing entry and returns true
crates/contract/src/tee/tee_state.rs Adds refresh_launcher_usage, clean_expired_launcher_images; propagates TTL through all reads and reverifications; adds a refresh test
crates/contract/src/v3_12_0_state.rs Adds OldTeeState / OldAllowedLauncherImage(s) borsh shadow + From impl stamping timestamps to now; adds a round-trip migration test
crates/contract/src/snapshots/...borsh_schema.snap, crates/contract/tests/snapshots/abi.snap Regenerated snapshots for the schema/ABI additions
crates/contract/tests/sandbox/{contract_configuration,upgrade_from_current_contract}.rs Updates sandbox configs with the two new fields
crates/mpc-attestation/src/attestation.rs New VerifiedAttestation::launcher_compose_hash()
crates/near-mpc-contract-interface/src/method_names.rs Adds CLEAN_EXPIRED_LAUNCHER_HASHES
crates/near-mpc-contract-interface/src/types/config.rs Adds the two new config fields to Config/InitConfig DTOs
crates/test-utils/src/contract_types.rs Adds new fields to dummy_config
docs/design/auto-remove-launcher-hashes-design.md Status → Implemented; notes participant-gating; open questions → decisions

Findings

Blocking (must fix before merge):

  • crates/contract/src/lib.rs:2014,2081init and init_running build the config with init_config.map(Into::into).unwrap_or_default() but never call Config::validate(). Only update_config (line 2409) validates. An operator can therefore initialize the contract with launcher_hash_unused_ttl_seconds < DEFAULT_EXPIRATION_DURATION_SECONDS, breaking the very safety invariant the validator was added to enforce ("a hash backing a valid participant attestation is never expired"). Please call config.validate() on both init paths and propagate the error (both are already #[handle_result]).

Non-blocking (nits, follow-ups, suggestions):

  • crates/mpc-attestation/src/attestation.rs:28 vs crates/contract/src/config.rs:113, crates/test-utils/src/contract_types.rs:18, crates/contract/tests/sandbox/contract_configuration.rs:106, crates/contract/tests/sandbox/upgrade_from_current_contract.rs:122, docs/design/auto-remove-launcher-hashes-design.md:34,76 — the actual constant DEFAULT_EXPIRATION_DURATION_SECONDS = 60*60*24 is 1 day, but new comments and the design doc call it "7 days". The test values (14 days) still satisfy the real bound so nothing breaks, but the comments are misleading — either drop the parenthetical or match the current constant. The safety-invariant argument in the design doc also relies on "at most 7d old" for participant attestations, which needs a matching update if the 1-day constant is authoritative.

  • crates/contract/src/tee/proposal.rs:269-274is_expired returns false on checked_add overflow (deadline unrepresentable ⇒ "never expires"). Intentional and safe here, but worth a one-line WHY comment given the arithmetic policy in engineering-standards — otherwise a future reader may "fix" it to a panic.

  • crates/contract/src/tee/proposal.rs:302 — the log! on re-vote ("launcher hash already in allowed list, refreshing") does not mention that the caller's add now returns true for this path. vote_add_launcher_hash logs \"launcher hash add result: {}\" (lib.rs:1486) which will read true even though no new entry was created. Consider tightening the log message (or returning an enum) so operator logs distinguish "newly added" from "refreshed".

  • crates/contract/src/tee/proposal.rs:296-305 — refreshing an existing entry updates added but leaves compose_hashes untouched. This is consistent with add_mpc_image_compose_hashes being the sole path for adding new compose hashes, but a one-line comment on the refresh branch would help — otherwise it looks like a re-vote should also "refresh" the compose set against the current MPC image list.

  • crates/contract/src/tee/tee_state.rs:337-351refresh_launcher_usage takes _authenticated_participant: &AuthenticatedParticipantId purely as a type-level capability token. That's fine and matches other patterns in this codebase, but the current docstring says "Requires an [AuthenticatedParticipantId] so a non-participant submission cannot keep a launcher hash alive." — worth explicitly calling out that the argument is intentionally unused so a future reader doesn't "clean up" the parameter.

  • crates/contract/src/lib.rs:795-839 — the authenticated_participant is computed before add_participant runs but consumed after. add_participant doesn't touch the participant set so this is safe today, but a comment noting the intentional ordering (or hoisting the refresh to a helper) would make the invariant less fragile.

  • crates/contract/src/tee/proposal.rs:362-381cleanup_expired recomputes is_expired per entry twice on the fast path (once in any, once in retain). Micro; entries list is tiny. Leaving as-is is fine.

⚠️ Issues found

Address review feedback:
- init/init_running now validate the config (not just update_config), so the
  contract cannot be initialized with launcher_hash_unused_ttl_seconds below
  the attestation validity window; add a regression test.
- dummy_config: give clean_expired_launcher_hashes_tera_gas a unique offset
  (was duplicating remove_non_participant_tee_verifier_votes_tera_gas).
Address non-blocking review comments:
- is_expired: note why overflow returns not-expired (never panic on a bogus timestamp)
- add(): note re-vote refreshes only the clock; compose hashes managed separately
- refresh_launcher_usage: note the AuthenticatedParticipantId is a capability token
@barakeinav1

barakeinav1 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review. Responses inline:

init and init_running build the config with init_config.map(Into::into).unwrap_or_default() but never call Config::validate(). Only update_config validates.

Fixed in d7c0c65 — both init paths now call config.validate() and propagate the error (both are #[handle_result]), with a regression test init_rejects_launcher_ttl_below_attestation_validity.

is_expired returns false on checked_add overflow ... worth a one-line WHY comment

Done (a8694aa): added a comment explaining the deadline is unrepresentably far off and we must never panic on a bogus timestamp.

refreshing an existing entry updates added but leaves compose_hashes untouched ... a one-line comment ... would help

Done (a8694aa): the re-vote branch now notes compose hashes are maintained separately via add_mpc_image_compose_hashes.

refresh_launcher_usage takes _authenticated_participant: &AuthenticatedParticipantId purely as a type-level capability token ... worth explicitly calling out that the argument is intentionally unused

Done (a8694aa): docstring now states it is a capability token with the value intentionally unused.

the log! on re-vote ... does not mention that the callers add now returns true ... Consider tightening the log message (or returning an enum)

Left as-is: the proposal-level log! already distinguishes "refreshing" from a new add; an enum return felt like over-engineering for a log line.

authenticated_participant is computed before add_participant runs but consumed after ... a comment noting the intentional ordering

cleanup_expired recomputes is_expired per entry twice on the fast path ... Micro; entries list is tiny. Leaving as-is is fine.

Left as-is — micro / the list is tiny, as you noted.

…and design doc

DEFAULT_EXPIRATION_DURATION_SECONDS was changed 7d -> 1d in #3626. Update the
stale '7 days' wording; reference the constant instead of hardcoding a
day-count in test comments to avoid future drift. The MPC docker-image grace
period (DEFAULT_TEE_UPGRADE_DEADLINE_DURATION_SECONDS) remains 7 days.
@barakeinav1

barakeinav1 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

the actual constant DEFAULT_EXPIRATION_DURATION_SECONDS = 60*60*24 is 1 day, but new comments and the design doc call it "7 days" ... either drop the parenthetical or match the current constant. The safety-invariant argument in the design doc also relies on "at most 7d old" ... which needs a matching update

Good catch — fixed in cb955fb. It was deliberately changed 7d → 1d in #3626, so the wording was stale. Updated the comments/design doc to "1 day", and to avoid this drift recurring I dropped the hardcoded day-count in the test comments (they now just reference the constant). The design-doc safety argument is now phrased around the constant (TTL >= DEFAULT_EXPIRATION_DURATION_SECONDS holds regardless of its exact value). The unrelated DEFAULT_TEE_UPGRADE_DEADLINE_DURATION_SECONDS (MPC docker-image grace) stays 7 days, which is still correct.

Comment thread crates/contract/src/tee/proposal.rs Outdated
Comment on lines +260 to +261
pub(crate) added: Timestamp,
pub(crate) last_attested: Timestamp,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn’t it be simpler to just have last_used instead of both added and last_attested? I don’t see why we need that distinction. last_used could then be refreshed both when re-voting for the launcher hash and when the attestation is used.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be simpler to just have last_used instead of both added and last_attested?

Agreed — collapsed to a single last_used (refreshed on both re-vote and participant attestation) in c435dc5.

For context on why I originally split them: I wanted to distinguish governance liveness (when the hash was voted in) from usage liveness (last participant attestation), so the all-expired fallback would deliberately retain the most-recently-voted-in hash. But the only readers are the expiry check (max, unchanged) and that fallback — and "keep the most-recently-used" is just as sensible there (arguably better). So the distinction isn't load-bearing; one field is simpler and equivalent.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oups sorry commented the same thing. Just noticed you had the same comment

Comment thread crates/contract/src/tee/proposal.rs Outdated
Comment on lines +260 to +261
pub(crate) added: Timestamp,
pub(crate) last_attested: Timestamp,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s a bit confusing to use the Timestamp type here while we use u64 in other places. We should probably align on one type unless there’s a specific reason not to:

// TODO(#1639): This timestamp can not come from the contract,
// but should be extracted from the certificate itself.
pub expiry_timestamp_seconds: u64,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit confusing to use the Timestamp type here while we use u64 in other places.

Kept Timestamp here: in this file it matches the sibling AllowedMpcDockerImage.added: Timestamp, and gives checked_add/now/Ord for free. The u64 you linked is in the mpc-attestation crate (raw unix seconds in the attestation DTO) — switching the launcher fields to u64 would make them inconsistent with their neighbor here. The broader u64-vs-Timestamp alignment across crates is a real but pre-existing, separate cleanup.

Comment thread crates/contract/src/tee/proposal.rs Outdated
/// Prepaid gas for a `remove_non_participant_tee_verifier_votes` call.
pub(crate) remove_non_participant_tee_verifier_votes_tera_gas: u64,
/// TTL after which a launcher image hash unused by any participant is evicted.
pub(crate) launcher_hash_unused_ttl_seconds: u64,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no test covering enlarging the launcher_hash_unused_ttl_seconds config while entries are stored: a bigger TTL should bring back an entry a smaller TTL had hidden, since filtering only hides (doesn't delete — only cleanup_expired does). Existing tests only advance time at a fixed TTL.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no test covering enlarging the launcher_hash_unused_ttl_seconds ... a bigger TTL should bring back an entry a smaller TTL had hidden

Good point — added enlarging_ttl_unhides_previously_expired_entry in c435dc5, asserting a larger TTL re-surfaces an entry a smaller TTL hid (filtering hides, only cleanup_expired deletes).

@SimonRastikian

Copy link
Copy Markdown
Contributor

The design doc [docs/design/auto-remove-launcher-hashes-design.md](https://github.com/near/mpc/blob/main/docs/design/auto-remove-launcher-hashes-design.md still says it's a Draft

@barakeinav1

Copy link
Copy Markdown
Contributor Author

The design doc [docs/design/auto-remove-launcher-hashes-design.md](https://github.com/near/mpc/blob/main/docs/design/auto-remove-launcher-hashes-design.md still says it's a Draft

I should have written "design had been reviewed (by @pbeza and @netrome) )

… simplify live_indices

Address review feedback:
- AllowedLauncherImage: replace added + last_attested with one last_used,
  refreshed on both re-vote and participant attestation. Expiry and the
  all-expired fallback (now newest-by-last_used) are unchanged in behavior.
- live_indices: drop the .expect()/empty-guard in favor of
  max_by_key(...).unwrap_or_default().
- Add a test that enlarging the TTL un-hides an entry a smaller TTL filtered
  (read-time filtering hides, never deletes).
- Regenerate borsh-schema snapshot for the field change.

@SimonRastikian SimonRastikian left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review only. Will do the second part most likely later today

Comment thread docs/design/auto-remove-launcher-hashes-design.md Outdated
Comment thread docs/design/auto-remove-launcher-hashes-design.md Outdated
Comment thread crates/contract/src/tee/proposal.rs Outdated
Comment on lines +269 to +271
// Overflow means the deadline is unrepresentably far in the future, so the
// entry is not expired. Never panic here: a bogus timestamp must not evict a hash.
None => false,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's interesting, not sure if I strongly like it or strongly dislike it. I guess this should be fine and cannot be called adversarially. or can it? If it can then better evict the hash (even on bogus timestamp)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not adversarial: last_used is stamped by the contract via Timestamp::now() (block time, seconds) — never user-supplied — so last_used + ttl cannot approach u64::MAX for ~centuries. Overflow is unreachable in practice, so returning "not expired" is safe (and we prefer never to evict a hash on an arithmetic edge). Kept as-is with the explanatory comment.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good them

@pbeza pbeza Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@barakeinav1 @SimonRastikian I think we should add a log! here. If this ever gets printed, it would be a red flag that something is very wrong. Without logging it, we might never catch it.

Comment thread crates/contract/src/tee/proposal.rs Outdated
Comment thread crates/contract/src/tee/proposal.rs Outdated
Comment thread crates/contract/src/tee/proposal.rs
Comment thread crates/contract/src/tee/proposal.rs Outdated
Comment thread crates/contract/src/tee/proposal.rs Outdated
Comment thread crates/contract/src/tee/proposal.rs Outdated
Comment thread crates/contract/src/tee/proposal.rs Outdated

@SimonRastikian SimonRastikian left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review only. Will do the second part most likely later today

…c to last_used

Address review feedback (SimonRastikian):
- add -> add_or_refresh returning enum AddOutcome { Added, Refreshed }; the
  bool return was always true. Thread the outcome through add_launcher_image
  and make vote_add_launcher_hash log added vs refreshed. (internal only; not
  in state/ABI)
- fix all_compose_hashes/launcher_hashes docstrings: they return live entries
  (non-expired, or the most-recently-used fallback when all expired)
- fix stale refresh_launcher_usage docstring (last_attested -> last_used)
- rewrite the design doc to the single last_used model (struct, expiry,
  mermaid, migration) and correct the stale 1-day attestation-validity wording
@jackson-harris-iii
jackson-harris-iii force-pushed the feat/auto-remove-launcher-hashes branch from 9cf1bd1 to 93c9ab5 Compare August 1, 2026 11:22
@claude

claude Bot commented Aug 1, 2026

Copy link
Copy Markdown

PR title type suggestion: This PR changes only configuration files, assets, and dependencies—no source code changes. The type prefix should probably be chore: instead of feat:.

Suggested title: chore: add fonts and configuration updates

@andrei-near
andrei-near force-pushed the feat/auto-remove-launcher-hashes branch from 93c9ab5 to 9cf1bd1 Compare August 1, 2026 16:05
…aming)

- TryFrom<InitConfig/Config> for Config now returns `Error` (via
  ConversionError::DataConversion) instead of `&'static str`, matching the
  other conversions in dto_mapping.rs; simplifies the init/init_running/
  update_config call sites.
- submit_participant_info mock arm: define refresh vars right before use and
  shadow the Option binding in the `if let`.
- Rename AllowedLauncherImages::newest_index -> latest_expiry_index.
- Name the magic timestamp/expiry constants in the expired-launcher test.
- Restore the pre-existing doc on get_allowed_launcher_hashes.
Follow-up to review: the launcher-TTL rejection test lacked the GWT
structure adopted across the rest of the new launcher tests.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.

Suppressed comments (1)

crates/test-utils/src/contract_types.rs:23

  • dummy_config is intended to generate distinct values per field, but clean_expired_launcher_hashes_tera_gas is set to value + 14, which duplicates verifier_tera_gas (also value + 14). This can mask DTO mapping/serialization issues because swapping those fields would still produce the same dummy config.
        clean_expired_launcher_hashes_tera_gas: value + 14,

…omise

Per review (gilcu3, pbeza): the MPC docker-image hashes are already cleaned
up inline in `reverify_and_cleanup_participants`, and the allowed launcher set
is a small in-memory `Vec`, so a detached-promise sweep is unnecessary
machinery. Evict expired entries inline right after the docker-hash cleanup.

Removes:
- the `#[private] clean_expired_launcher_hashes` method + its promise spawn in
  `verify_tee` and the `CLEAN_EXPIRED_LAUNCHER_HASHES` method-name constant
- the `clean_expired_launcher_hashes_tera_gas` config field (Config + InitConfig
  DTOs, defaults, mappings, migration shadow, test fixtures)
- `TeeState::{clean_expired_launcher_images, has_expired_launcher_images}` and
  `AllowedLauncherImages::has_expired`

Regenerates the borsh-schema and ABI snapshots (only these removals). Updates
the design doc (inline eviction is now the chosen approach; the detached-promise
sweep moves to Alternatives considered).
…up_participants

The eviction logic (cleanup_expired) was unit-tested in isolation, but nothing
exercised it through reverify_and_cleanup_participants. Assert on raw storage
(expires_at_secs) rather than the read path, which already filters expired
entries and would pass regardless.
gilcu3
gilcu3 previously approved these changes Aug 4, 2026

@gilcu3 gilcu3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for all the fixes!

Comment on lines +9 to +14
/// The largest representable timestamp. Used as the saturating result when adding a
/// TTL to `now()` would overflow, so a bogus timestamp or enormous TTL yields an
/// entry that never expires rather than panicking.
pub(crate) const MAX: Self = Self {
duration_since_unix_epoch: Duration::MAX,
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we could just drop the comment. Usually comments explaining trivial things are not useful to the reader.

Comment thread crates/contract/README.md
Comment on lines -327 to +328
| `allowed_launcher_image_hashes()` | Returns all currently allowed launcher image hashes. | `Vec<LauncherImageHash>` | TBD | TBD |
| `allowed_launcher_compose_hashes()` | Returns all currently allowed launcher compose hashes (derived from launcher + MPC image pairs). | `Vec<LauncherDockerComposeHash>` | TBD | TBD |
| `allowed_launcher_image_hashes()` | Returns the non-expired allowed launcher image hashes (the most-recently-used entry only when all are expired). | `Vec<LauncherImageHash>` | TBD | TBD |
| `allowed_launcher_compose_hashes()` | Returns the non-expired allowed launcher compose hashes (derived from launcher + MPC image pairs; the most-recently-used entry only when all are expired). | `Vec<LauncherDockerComposeHash>` | TBD | TBD |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am slightly concerned about us losing the capability of observing the expired hashes. Do we have any other way of doing so?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point — filed #4047 to expose the launcher hashes' expiry off-chain (mirroring what allowed_docker_image_hashes() already does), since it's a small API decision rather than in-scope here.

@gilcu3

gilcu3 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

@claude review

(@barakeinav1 this is just to avoid missing something, reviews are more stringent lately)

@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown

Pull request overview

Usage-based expiry for the launcher-image allowlist, replacing unanimous-vote-only removal for routine rotation. Each AllowedLauncherImage stores expires_at, stamped now + TTL at every write site (vote-in, re-vote, and a successful attestation by a current participant, gated by an AuthenticatedParticipantId capability token). Reads filter expires_at < now with a latest-expiry fallback so the set is never empty; reverify_and_cleanup_participants evicts expired entries inline next to the existing MPC docker-image cleanup. A v3_13_0_state shadow migrates the timestamp-less layout.

The core mechanics check out. I verified: OldTeeState's field order matches the real TeeState (tee_state.rs:79-90) with expires_at appended last, so old bytes decode; From<OldConfig> uses ..Config::default() so the new field is covered; store_verified_attestation already rejects a TLS key owned by another account, so the post-store refresh_launcher_usage can only touch the caller's own entry; expired entries cannot be resurrected by refresh, since every refresh site runs only after verification against the filtered set; DEFAULT_EXPIRATION_DURATION_SECONDS is 7 days on main, matching the design doc; the node needs no change (monitor_allowed_launcher_compose_hashes reads the view, so same filtering; validate_and_submit_remote_attestation only warns on a local mismatch).

Changes:

  • AllowedLauncherImage::{new, is_expired} + expires_at; expiry_from_now saturates at the new Timestamp::MAX instead of panicking.
  • add -> add_or_refresh returning AllowedLauncherImageInsertion; new latest_expiry_index, non_expired_or_newest_indices, refresh, cleanup_expired, from_entries; filtered reads.
  • TeeState::refresh_launcher_usage gated on AuthenticatedParticipantId; inline cleanup_expired(); add_launcher_image takes a TTL.
  • New Config.launcher_hash_unused_ttl_seconds (14d) + Config::validate(), folded into TryFrom for both config DTOs so init / init_running / update_config cannot skip it.
  • Refresh wired into both attestation paths with negative gate tests; migration shadows + tests; VerifiedAttestation::launcher_compose_hash(); regenerated snapshots; README, operator guide, design doc updated.

Reviewed changes

Per-file summary
File Description
crates/contract/src/tee/proposal.rs expires_at, is_expired, expiry_from_now, add_or_refresh + insertion enum, index/refresh/cleanup helpers, from_entries, test-only expires_at_secs, filtered reads + tests
crates/contract/src/tee/tee_state.rs refresh_launcher_usage; inline cleanup_expired(); add_launcher_image signature; eviction/refresh/expired-rejection tests
crates/contract/src/lib.rs TTL threaded to write sites; participant-gated refresh on mock + dstack paths; config validation on init paths and update_config; new tests
crates/contract/src/config.rs, dto_mapping.rs New TTL field + Config::validate(); From -> TryFrom conversions that validate
crates/contract/src/primitives/time.rs Adds Timestamp::MAX for the saturating expiry
crates/contract/src/v3_13_0_state.rs OldAllowedLauncherImage(s) / OldTeeState shadows + From stamping expires_at; migration tests
crates/contract/src/tee/test_utils.rs add_launcher_image call updated for the TTL argument
crates/mpc-attestation/src/attestation.rs VerifiedAttestation::launcher_compose_hash() for dstack + WithConstraints mock
crates/near-mpc-contract-interface/src/types/config.rs, crates/test-utils/src/contract_types.rs New DTO field + test helpers
crates/contract/tests/sandbox/*.rs, /snapshots/.snap Sandbox config payloads and regenerated borsh/ABI snapshots
crates/contract/README.md, docs/*.md Launcher views describe expiry filtering; design doc -> Implemented; operator guide covers auto-expiry

Findings

Blocking (must fix before merge):

  • crates/contract/src/lib.rs:2516 (with update.rs:79 and :186) — an invalid config proposal is silently consumed and wipes every other pending proposal. update_config can now fail (Config::try_from(...).unwrap_or_else(|e| env::panic_str(...))), but nothing validates the config at proposal time: TryFrom<ProposeUpdateArgs> for Update (update.rs:79-102) only checks code/config exclusivity, and propose_update (lib.rs:1335) just calls args.try_into()?. A participant can propose launcher_hash_unused_ttl_seconds: 0, reach threshold, and then do_update (update.rs:186-190) removes the entry and calls entries.clear() / vote_by_participant.clear() in the caller's receipt, before spawning update_config as a separate receipt (update.rs:208-213). That receipt panics; the parent receipt's writes are already committed and vote_update returns Ok(true) (lib.rs:1425-1429). Net effect: config unchanged, every other pending proposal and vote destroyed, success reported. This also defeats the standards' rationale for contract panics ("panicking ensures no side-effects happen in the transaction", docs/engineering-standards.md:25) — here the side effects are in a different receipt. update_config was infallible before this PR, so the failure mode is new. Suggested fix in TryFrom<ProposeUpdateArgs> for Update:

    (None, Some(config)) => {
        // Reject unusable configs at proposal time: `update_config` runs in its own
        // receipt, so a late failure cannot roll back `do_update`.
        let _: crate::config::Config = config.clone().try_into()?;
        Update::Config(config)
    }
  • Test naming/structure — CLAUDE.md states new tests must use <system_under_test>__should_<assertion> with // Given / // When / // Then. Not the case for crates/contract/src/tee/proposal.rs:816,843,864,882,913 (refresh_keeps_entry_alive_past_ttl, expired_entries_are_filtered_from_reads, newest_fallback_when_all_expired, cleanup_expired_removes_expired_but_keeps_one, re_add_refresh_resets_expires_at_and_keeps_alive — no __should_ form, no Given/When/Then), crates/contract/src/lib.rs:4093, crates/contract/src/tee/tee_state.rs:1307,1689, crates/contract/src/v3_13_0_state.rs:264,365. All added by this PR; the same files already follow the convention (tee_state.rs:1765, and the pre-existing stamp_expiry_on_legacy_mocks__should_make_valid_mock_cleanable). Mechanical, but it was reported as done in an earlier round.

Non-blocking (nits, follow-ups, suggestions):

  • crates/contract/src/v3_13_0_state.rs:264 — the migration test serializes OldTeeState and deserializes it as OldTeeState, so it proves self-consistency but cannot detect divergence from the real 3.13.0 TeeState layout (a reordered/omitted field round-trips fine). The archived-wasm path doesn't cover it either: tests/sandbox/upgrade_to_current_contract.rs builds old state via execute_key_generation_and_add_random_state (tests/sandbox/common.rs:586), which never calls vote_add_launcher_hash — so migration only ever decodes an empty entries vec, byte-identical either way. Adding one vote_add_launcher_hash there turns this into real coverage. (I checked the field order by hand and it matches, so this is a coverage gap, not a defect.)

  • crates/contract/src/config.rs:86 and crates/near-mpc-contract-interface/src/types/config.rs:116 — "TTL after which a launcher image hash unused by any participant is evicted" reads as if the TTL applied retroactively, but with stored expires_at a config change only takes effect on an entry's next stamp. The design doc says this; the ABI/config doc an operator actually reads doesn't. Worth one clause, especially since the earlier enlarging_ttl_unhides_previously_expired_entry test (and the behavior it pinned) is gone with this refactor.

  • crates/contract/src/tee/tee_state.rs:369refresh_launcher_usage drops the bool from refresh. A false means the just-verified attestation references a compose hash absent from the allowed set, i.e. an inconsistency between verify_and_store_* and the allowlist — currently silent. A log! on that miss would surface it (the "unknown TLS key" early return above is genuinely expected).

  • crates/contract/README.md:328@gilcu3's 2026-08-04 question about losing visibility into expired hashes is still open. Supporting detail: allowed_docker_image_hashes() returns Vec<AllowedMpcDockerImageHash> including the eviction expiry (README:326), while allowed_launcher_image_hashes() returns bare hashes and now hides expired ones, so expires_at is unobservable off-chain — expires_at_secs exists but is #[cfg(test)] (proposal.rs:762).

  • docs/tee-lifecycle.md:233,248-249 and docs/securing-mpc-with-tee-design-doc.md:686-696 describe launcher governance purely as add-by-threshold / remove-by-unanimity. Nothing there is now false, so not drift in the strict sense, but both enumerate the launcher lifecycle and a one-line pointer to auto-expiry would keep them aligned with the operator guide and README you did update.

⚠️ Issues found

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.

Suppressed comments (3)

docs/running-an-mpc-node-in-tdx-external-guide.md:1829

  • This wording reads as if eviction from storage happens immediately once a launcher digest becomes unused. In the implementation, expiry is enforced by filtering (so it becomes unusable immediately), but storage is reclaimed later during verify_tee housekeeping. Consider clarifying the distinction to avoid confusing operators.
An unused launcher manifest digest now auto-expires after the configured TTL (`launcher_hash_unused_ttl_seconds`, default 14 days) and is evicted automatically once no node has attested with it for that window, so no vote is needed for routine rotation. The unanimous `vote_remove_launcher_hash` is only needed to remove a still-valid digest *immediately* (before its TTL lapses), for example a compromised launcher.

crates/contract/src/tee/proposal.rs:433

  • The comment says this keeps the "most-recently-used" entry, but the implementation keeps the entry with the latest expires_at. If the configured TTL ever changes between stamps, the latest expires_at may not correspond to most-recently-used. Suggest rewording to match the actual selection criterion.
        } else if let Some(newest) = self.latest_expiry_index() {
            // All expired: keep only the most-recently-used entry.
            self.entries.swap(0, newest);
            self.entries.truncate(1);

docs/running-an-mpc-node-in-tdx-external-guide.md:1739

  • This sentence implies the view call physically evicts expired launcher digests. allowed_launcher_image_hashes only filters out expired entries at read time; actual storage eviction happens during TEE housekeeping (e.g. verify_teereverify_and_cleanup_participants). Please reword to avoid suggesting immediate eviction on query.

This issue also appears on line 1829 of the same file.

The contract method is named `allowed_launcher_image_hashes` for historical reasons, but the values returned are manifest digests. The query returns only non-expired digests; digests that have aged out past their TTL are hidden and auto-evicted.

`update_config` became fallible in this PR (it validates the launcher TTL),
but `do_update` clears all pending proposals/votes in the caller's receipt
before spawning `update_config` as a separate receipt. An invalid config
reaching threshold therefore wiped every other pending proposal while the
config change silently failed and `vote_update` still reported success.

Validate the config in `TryFrom<ProposeUpdateArgs> for Update`, so an
unusable config is rejected up front and never reaches `do_update`.
- Rename PR-added tests to `<sut>__should_<assertion>` and add Given/When/Then,
  per the engineering standards.
- Vote a launcher hash into the 3.13.0 sandbox state before upgrading, so the
  launcher-image migration decodes a non-empty `entries` vec off the real old
  layout (previously only the empty-vec path was exercised) and assert the hash
  survives the upgrade.
…piry

- Note on `launcher_hash_unused_ttl_seconds` (config + DTO) that a change
  applies on an entry's next stamp, not retroactively.
- Point tee-lifecycle and securing-mpc docs at the auto-removal design.
@barakeinav1

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review — addressed below.

Blocking — invalid config proposal wipes every other pending proposal

update_config can now fail (...) but nothing validates the config at proposal time (...) Net effect: config unchanged, every other pending proposal and vote destroyed, success reported.

Good catch, confirmed and fixed in 94a5fc5. TryFrom<ProposeUpdateArgs> for Update now validates the config, so an unusable config is rejected at propose time and never reaches do_update. Added update_try_from__should_reject_invalid_config_at_propose_time.

Blocking — test naming / Given-When-Then

new tests must use <system_under_test>__should_<assertion> with // Given / // When / // Then

Done in ef6386d — renamed the PR-added tests to the __should_ form and added Given/When/Then.

Non-blocking — migration test only proves self-consistency / sandbox decodes an empty entries vec

Adding one vote_add_launcher_hash there turns this into real coverage.

Done in ef6386dpropose_upgrade_from_production_to_current_binary now votes a launcher hash into the real 3.13.0 sandbox state before upgrading and asserts it survives migration, so the non-empty launcher layout is decoded off the real old bytes.

Non-blocking — config doc reads as retroactive
Done in 118fbee — the launcher_hash_unused_ttl_seconds doc (config + DTO) now states the change applies on an entry's next stamp, not retroactively.

Non-blocking — refresh_launcher_usage drops the bool from refresh
Skipping: refresh is only called when the attestation carries a launcher hash, and a just-verified attestation's hash is always in the allowed set — so false is a can't-happen case and a log! there would be dead observability. Happy to revisit if you see a path where it can legitimately miss.

Non-blocking — no off-chain visibility into expired hashes (@gilcu3's question)
Tracked as a follow-up: #4047. Kept as its own issue since exposing expires_at off-chain is a small API/product decision rather than in-scope for this PR.

Non-blocking — tee-lifecycle.md / securing-mpc-with-tee-design-doc.md
Done in 118fbee — both now point at the auto-removal design.

- proposal.rs: the all-expired fallback keeps the entry with the latest
  expiry, not the "most-recently-used" one (they differ if the TTL changed
  between stamps); fix the comment and local binding name.
- external TDX guide: the launcher view only filters expired digests at read
  time; physical removal happens during routine `verify_tee`. Reword both
  passages so they don't read as immediate on-query eviction.
@barakeinav1
barakeinav1 requested a review from gilcu3 August 4, 2026 10:58
Comment on lines 76 to +92
Config(near_mpc_contract_interface::types::Config),
}

impl TryFrom<ProposeUpdateArgs> for Update {
type Error = Error;

fn try_from(value: ProposeUpdateArgs) -> Result<Self, Self::Error> {
let ProposeUpdateArgs { code, config } = value;
let update = match (code, config) {
(Some(contract), None) => Update::Contract(contract),
(None, Some(config)) => Update::Config(config),
(None, Some(config)) => {
// Reject unusable configs at proposal time: `update_config` runs in its own
// receipt, so a validation failure at apply time cannot roll back `do_update`
// (which has already cleared the pending proposals in the caller's receipt).
let _: crate::config::Config = config.clone().try_into()?;
Update::Config(config)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this really tells me that the correct change is to change the type in :

pub enum Update {
    Contract(Vec<u8>),
    Config(near_mpc_contract_interface::types::Config),
}

to use the internal type instead. Could be done in a follow up if you agree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Auto-remove unused launcher image hashes from the contract

5 participants